Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start accounting for notificationPreferences in the unread indicator for Chat Rooms #3766

Merged
merged 4 commits into from
Jun 29, 2021

Conversation

yuwenmemon
Copy link
Contributor

@yuwenmemon yuwenmemon commented Jun 25, 2021

@jasperhuangg please review

Details

When users have notificationPreferences in E.Chat for a given, room (comes live with https://github.com/Expensify/Expensify/issues/161785 cc @jasperhuangg) - follow them for push notifications within the app.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/161781

Tests/QA

  1. Set your notification preferences for a policy's announce room to "always" by running the following JS in E.Web (User 1):
p = Policy.getCurrent();
announceID = p.policy.chatReportIDAnnounce
API._performPOSTRequest("Report_UpdateNotificationPreference", {
    reportID: announceID,
    notificationPreference: 'always'
});
  1. For Phones, keep E.Chat running in the background for User 1, for Web/Desktop, have the tab or window open but not focused.
  2. Open up E.Chat in another client from a different account that's also in the same policy (User 2)
  3. From that account send 4 messages in the #announce room for that policy.
  4. Make sure that the app indicator for the client User 1 is in shows 4

  1. Set your notification preferences for a policy's announce room to "daily" by running the following JS in Web Expensify (OldDot) (User 1):
p = Policy.getCurrent();
announceID = p.policy.chatReportIDAnnounce
API._performPOSTRequest("Report_UpdateNotificationPreference", {
    reportID: announceID,
    notificationPreference: 'daily'
});
  1. For Phones, keep E.Chat running in the background for User 1, for Web/Desktop, have the tab or window open but not focused.
  2. Open up E.Chat in another client from a different account that's also in the same policy (User 2)
  3. From that account send a message in the #announce room for that policy.
  4. Make sure that the app indicator for the client User 1 is using shows 1 or is simply blank but present.

  1. Set your notification preferences for a policy's announce room to "mute" by running the following JS in Web Expensify (OldDot) (User 1):
p = Policy.getCurrent();
announceID = p.policy.chatReportIDAnnounce
API._performPOSTRequest("Report_UpdateNotificationPreference", {
    reportID: announceID,
    notificationPreference: 'mute'
});
  1. For Phones, keep E.Chat running in the background for User 1, for Web/Desktop, have the tab or window open but not focused.
  2. Open up E.Chat in another client from a different account that's also in the same policy (User 2)
  3. From that account send a message in the #announce room for that policy.
  4. Make sure that you do not see any indicator update on the app icon for User 1

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android (Not Applicable)

Screenshots

Web

Screen Shot 2021-06-25 at 4 24 15 PM

Mobile Web

N/A

Desktop

Screen Shot 2021-06-28 at 4 12 17 PM

iOS

Screen Shot 2021-06-28 at 4 55 24 PM

Android

Not Applicable

@yuwenmemon yuwenmemon requested a review from a team as a code owner June 25, 2021 23:13
@yuwenmemon yuwenmemon self-assigned this Jun 25, 2021
@MelvinBot MelvinBot requested review from thienlnam and removed request for a team June 25, 2021 23:14
@jasperhuangg
Copy link
Contributor

Hey @yuwenmemon! I can't seem to get this to work on Desktop when testing 'always'. I'm pretty sure I've followed the right steps, but the indicator always shows up as 1 regardless of how many messages I send, am I missing anything? Here's a screen capture:

demo.mp4

@yuwenmemon yuwenmemon changed the title Start accounting for notificationPreferences in the LHN for Chat Rooms Start accounting for notificationPreferences in the unread indicator for Chat Rooms Jun 28, 2021
@yuwenmemon
Copy link
Contributor Author

@jasperhuangg yep, unfortunately that's expected behavior for Desktop as the Electron API fails otherwise. See my QA step here:

Make sure that the app indicator for the client User 1 is using shows 1 or is simply blank but present.

and this comment here:
https://github.com/Expensify/Expensify.cash/pull/3766/files#diff-9276f078b750aa322d09bda48c79eb8034941df5b0f0f0c4f8b898cf9c652c41R244-R247

thienlnam
thienlnam previously approved these changes Jun 28, 2021
Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for entertaining my questions! Changes look good

@yuwenmemon
Copy link
Contributor Author

Didn't see that you referring to an always preference @jasperhuangg - my bad. Just tested that again, not seeing what you're seeing. I have the always user signed in on the desktop app in this scenario:
Kapture 2021-06-28 at 20 31 44

Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great and tests well :)

@jasperhuangg jasperhuangg merged commit 1bfa5d2 into main Jun 29, 2021
@jasperhuangg jasperhuangg deleted the yuwen-LHNPrefs branch June 29, 2021 04:20
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.74-1🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@isagoico
Copy link

@jasperhuangg @yuwenmemon Not sure if this is testable for us at the moment. I tried creating a workspace and adding members but I'm stuck on this page. It won't let me add members or create the chat rooms.
image

@yuwenmemon
Copy link
Contributor Author

@isagoico can you add those folks to a policy in the "normal" Expensify web app?

@isagoico
Copy link

I created the workspace on an account and was unable to find the Policy in the classic app to add the members there. Not sure if I'm missing anything.
image

@yuwenmemon
Copy link
Contributor Author

@isagoico perhaps just to keep things simple can you create the policy and add the members via the classic app?

@isagoico
Copy link

isagoico commented Jul 1, 2021

mmm I tried creating the policy in classic app and adding the members there and the workspace does not appear in e.cash. This is what I've tried, let me know if I'm missing something:

  1. Create workspace in e.cash
  2. Navigated to e.com to find the policy
  3. Policy is not displayed in e.com
  4. Tried accessing the policy via de Policy ID number in e.com (did not work)

Since above didn't work

  1. I created a policy in e.com in a account that didn't have any previous policies
  2. Added some members to the policy
  3. Checked e.cash if I could see the workspace, nothing was displayed

@yuwenmemon
Copy link
Contributor Author

I guess I'm a bit confused as to why you're looking for a workspace. We're not testing workspaces in this PR, we want to test the #announce room.

@isagoico
Copy link

isagoico commented Jul 1, 2021

OH I totally thought that the rooms like #announce were created when a workspace was set up. Not sure how to access the #announce room then 🤔 should it automatically appear on any account?

@yuwenmemon
Copy link
Contributor Author

Try searching for the policy name OR announce in the E.Chat app:
Screen Shot 2021-07-01 at 2 23 51 PM
Screen Shot 2021-07-01 at 2 23 58 PM

@isagoico
Copy link

isagoico commented Jul 2, 2021

That worked! Thank you so much for the clarification, I was completely of the loop on how to access the rooms.
I tried following the steps and got this errors when running the snippets
image
Also when sending the messages, no notifications were received on the User 2 device (in my case it was my android phone)

@yuwenmemon
Copy link
Contributor Author

@isagoico You'll need to run that snippet in the classic app.

There's no applicable test for this for Android, so that makes sense.

@isagoico
Copy link

isagoico commented Jul 5, 2021

Tested this and it was a pass:

  • First scenario:

Got notifications for each message in this case 4
image

  • Second scenario:

I got no notification but I did have the NEW! indicator.

  • Third scenario:

No notification or NEW! indicator was displayed.
image

@isagoico isagoico mentioned this pull request Jul 5, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants